Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

txnbuild: Updates package with Protocol 14 changes. #3048

Merged
merged 5 commits into from
Sep 25, 2020
Merged

txnbuild: Updates package with Protocol 14 changes. #3048

merged 5 commits into from
Sep 25, 2020

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Sep 22, 2020 •
edited
Loading

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Basically, I ran the following commands (simplified)

  • rsync -c -r -u internal/txnbuild ../../
  • find & replace "/services/horizon/internal/txnbuild" with "/txnbuild"
  • rm -rf internal/txnbuild

All integration & unit tests pass.

Why

We want to make our Protocol 14 updates visible.

Known limitations

Because of the -u flag on the rsync command, I only copied new[er] files. This means that if someone updated txnbuild with a change after we updated the same file inside of internal/txnbuild, it wouldn't get copied. I don't think this is the case anywhere and tried confirming with diff, but a sanity check would be wonderful.

Questions:

  • Do we need to update the CHANGELOG.md?
  • Or make version bumps anywhere

@Shaptic Shaptic self-assigned this Sep 22, 2020
@cla-bot cla-bot bot added the cla: yes label Sep 22, 2020
@Shaptic
Copy link
Contributor Author

Shaptic commented Sep 22, 2020 •
edited
Loading

Okay wait this is super incomplete, please don't review yet. was misreading the diff

I still think this is fine, as long as there were no changes to the same file in both txnbuild and internal/txnbuild. It looks like only #2967 was introduced during this time, and I've verified that those files still match.

abuiles
abuiles suggested changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@abuiles abuiles left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heads up, you are removing a bunch of valid files (they were removed from the internal txnbuild to make static check happy, but we shouldn't be removing files).

@Shaptic Shaptic linked an issue Sep 24, 2020 that may be closed by this pull request
Protocol 14 support #3035
Closed
@2opremio
Copy link
Contributor

2opremio commented Sep 25, 2020 •
edited
Loading

@Shaptic If you want to be 100% sure, it would worth trying out https://stackoverflow.com/a/5190916 (in the example the use a directory from a remote repo, but I think it can be easily adapter to two directories in the same repo)

@Shaptic Shaptic merged commit b0a2cdd into stellar:protocol-14 Sep 25, 2020
@Shaptic Shaptic deleted the txnbuild-update branch October 28, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tamirms tamirms tamirms approved these changes

@abuiles abuiles abuiles approved these changes

@2opremio 2opremio Awaiting requested review from 2opremio

Assignees

@Shaptic Shaptic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Protocol 14 support
4 participants
@Shaptic @2opremio @abuiles @tamirms